Skip to content

Reduce size of slow testcase in std.algorithm.setops#6280

Merged
dlang-bot merged 1 commit intodlang:masterfrom
FeepingCreature:fix/reduce-size-of-cartesian-tests
Mar 15, 2018
Merged

Reduce size of slow testcase in std.algorithm.setops#6280
dlang-bot merged 1 commit intodlang:masterfrom
FeepingCreature:fix/reduce-size-of-cartesian-tests

Conversation

@FeepingCreature
Copy link
Contributor

@FeepingCreature FeepingCreature commented Mar 15, 2018

This test takes an unfeasibly large amount of time to run. (I've never managed to run it to completion.)

The only reason why it appears to pass is that it completes instantly, because canFind never evaluates the Cartesian range at all due to DMD bug #12486. When I fixed it in DMD PR dlang/dmd#8013, the test started timing out.

By the way, how do I get reviewers for that DMD PR?

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

assert(canFind(N4, tuple(1, 2, 3, 4)));
assert(canFind(N4, tuple(4, 3, 2, 1)));
assert(canFind(N4, tuple(10, 31, 7, 12)));
assert(canFind(N4, tuple(10, 3, 7, 2)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better could that line be removed outright? Does it cover anything not already tested by

assert(canFind(N4, tuple(1, 2, 3, 4)));
assert(canFind(N4, tuple(4, 3, 2, 1)));

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess "larger numbers than 1 in every dimension"?

Copy link
Member

@quickfur quickfur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@quickfur
Copy link
Member

Though, BTW, a more fundamental problem here is how variadic cartesianProduct is implemented. The current implementation for infinite ranges causes an exponential number of template expansions, which will quickly cause system RAM to run out as the number of arguments grow. There ought to be a way of implementing it such that it's less template-heavy.

@dlang-bot dlang-bot merged commit afbb382 into dlang:master Mar 15, 2018
@FeepingCreature FeepingCreature deleted the fix/reduce-size-of-cartesian-tests branch March 15, 2018 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants